#!/bin/bash
# Get PR details and file list
gh pr view 16 --repo Postgres-Extensions/cat_tools --json title,body,files,additions,deletions,commits
echo "---FILES---"
gh pr diff 16 --repo Postgres-Extensions/cat_tools

#!/bin/bash
set -e

# First, gather file contents for accurate line numbers
gh api repos/Postgres-Extensions/cat_tools/pulls/16/files --jq '.[].filename' | head -40

# Get the PR's head SHA for the review
gh api repos/Postgres-Extensions/cat_tools/pulls/16 --jq '.head.sha'

#!/bin/bash
set -e

# Get line numbers of key issues in files by looking at file content
# 1. routine__.out - error in expected output
echo "=== test/expected/routine__.out ==="
cat -n test/expected/routine__.out

echo ""
echo "=== README.asc supported versions ==="
grep -n "Supported\|9\.\|12\|Postgres" README.asc | head -20

echo ""
echo "=== HISTORY.asc TODO ==="
grep -n "TODO\|Changes\|0\.3\.0" HISTORY.asc | head -15

echo ""
echo "=== relation__kind trailing whitespace in cat_tools.sql.in ==="
grep -n "pg_catalog.pg_class $\|pg_class $" sql/cat_tools.sql.in | head -10

echo ""
echo "=== relation__kind mapping comments in cat_tools--0.3.0.sql.in ==="
grep -n "but mapped\|composite type\|materialized view\|foreign table" sql/cat_tools--0.3.0.sql.in | head -20

echo ""
echo "=== REVOKE in 0.2.2--0.3.0 upgrade script ==="
grep -n "REVOKE\|GRANT.*function__arg_to\|GRANT.*function__drop" sql/cat_tools--0.2.2--0.3.0.sql.in

echo ""
echo "=== enum_mapping.sql line 65 area ==="
sed -n '60,70p' test/helpers/enum_mapping.sql

echo ""
echo "=== routine__.sql - enum_mapping.sql call area ==="
grep -n "enum_mapping\|sample_char\|sample_text\|kind " test/sql/routine__.sql | head -30

#!/bin/bash
set -e

SHA="434ef45ba69986a6cb4561302ea12a0067dca115"

# Verify specific line numbers in the upgrade script
echo "=== upgrade script lines 165-172 ==="
sed -n '165,172p' sql/cat_tools--0.2.2--0.3.0.sql.in

echo ""
echo "=== routine__.out full ==="
cat test/expected/routine__.out

echo ""
echo "=== enum_mapping.sql lines 60-70 with numbers ==="
sed -n '60,70p' test/helpers/enum_mapping.sql | cat -A

echo ""
echo "=== README.asc lines 20-26 ==="
sed -n '20,26p' README.asc

echo ""
echo "=== HISTORY.asc lines 14-20 ==="
sed -n '14,20p' HISTORY.asc

echo ""
echo "=== cat_tools.sql.in lines 1610-1632 (trailing space) ==="
sed -n '1610,1632p' sql/cat_tools.sql.in | cat -A

echo ""
echo "=== cat_tools--0.3.0.sql.in lines 380-410 (relkind mapping) ==="
sed -n '380,412p' sql/cat_tools--0.3.0.sql.in

echo ""
echo "=== test/build/upgrade.sql ==="
cat -n test/build/upgrade.sql

echo ""
echo "=== cat_tools--0.2.2--0.3.0.sql.in lines 255-175 - check REVOKE near GRANT ==="
grep -n "REVOKE\|GRANT" sql/cat_tools--0.2.2--0.3.0.sql.in

#!/bin/bash
set -e

SHA="434ef45ba69986a6cb4561302ea12a0067dca115"
REPO="Postgres-Extensions/cat_tools"
PR=16

# Submit the full review with inline comments
gh api \
  repos/$REPO/pulls/$PR/reviews \
  --method POST \
  --field commit_id="$SHA" \
  --field event="REQUEST_CHANGES" \
  --field body="## Review Summary

This is a substantial and well-structured release — the new enum taxonomy for `pg_proc` fields, the relation helper functions, the cross-version CI matrix, and the dynamic permissions test suite are all strong improvements.

There is one **blocking** issue (a committed test failure in `routine__.out`) and several smaller bugs/doc gaps that should be addressed before merge.

### Blocking
- **`test/expected/routine__.out` contains a live psql ERROR** — the test suite is failing as committed (see inline comment).

### Bugs
- Missing \`REVOKE EXECUTE FROM PUBLIC\` in the 0.2.2→0.3.0 upgrade script before the GRANTs on \`_cat_tools\` helpers.
- \`test/build/upgrade.sql\` wraps the upgrade in \`BEGIN/ROLLBACK\`, so \`ALTER EXTENSION cat_tools UPDATE\` never actually commits; there is no post-update version assertion.

### Documentation
- \`HISTORY.asc\` 0.3.0 **Changes** section is still a \`// TODO\` placeholder.
- \`README.asc\` still states "Works on Postgres 9.3 and above" but PG10/11 support was dropped in this PR.

### Style/minor
- Trailing whitespace in the SQL body strings for \`relation__is_temp\` and \`relation__is_catalog\`.
- Confusing backward-compat comments on the relkind CASE mapping." \
  --field "comments[][path]=test/expected/routine__.out" \
  --field "comments[][line]=4" \
  --field "comments[][side]=RIGHT" \
  --field "comments[][body]=**Blocking: committed test failure.**

Lines 4–8 are a verbatim psql \`ERROR\`, not a passing TAP line. The test file produced this error, was captured, and committed as the expected output — which means \`pg_regress\` will accept it as \"passing\" only if the real run emits the identical error. Any fix to \`enum_mapping.sql\` will break CI; any PG version that renders a different error message will also break CI.

Root cause: \`enum_mapping.sql\` line 62 calls
\`\`\`sql
:s.routine__:kind(:'sample_char')
\`\`\`
When \`kind=type\` and \`sample_char='f'\`, \`:'sample_char'\` is an **untyped string literal**. PostgreSQL cannot choose between the \`routine_prokind\` overload and the implicit \`CREATE CAST (\"char\" AS cat_tools.routine_prokind) WITH INOUT AS IMPLICIT\` path, so it raises \`42725: function is not unique\`.

Fix: cast to \`\"char\"\` explicitly in enum_mapping.sql:
\`\`\`sql
:s.routine__:kind(:'sample_char'::\"char\")
\`\`\`
Then regenerate the expected output." \
  --field "comments[][path]=sql/cat_tools--0.2.2--0.3.0.sql.in" \
  --field "comments[][line]=168" \
  --field "comments[][side]=RIGHT" \
  --field "comments[][body]=The install script (\`cat_tools--0.3.0.sql.in\`) explicitly revokes PUBLIC execute on these two helpers before granting to \`cat_tools__usage\`, but the upgrade script omits the \`REVOKE\`. As a result, users upgrading from 0.2.2 to 0.3.0 will have \`PUBLIC\` execute access on \`_cat_tools.function__arg_to_regprocedure\` and \`_cat_tools.function__drop_temp\`, defeating the defense-in-depth that was added in the last commit of this PR.

Add before the two \`GRANT\` lines:
\`\`\`sql
REVOKE EXECUTE ON FUNCTION _cat_tools.function__arg_to_regprocedure(text, text, text) FROM PUBLIC;
REVOKE EXECUTE ON FUNCTION _cat_tools.function__drop_temp(pg_catalog.regprocedure, text) FROM PUBLIC;
\`\`\`" \
  --field "comments[][path]=test/build/upgrade.sql" \
  --field "comments[][line]=21" \
  --field "comments[][side]=RIGHT" \
  --field "comments[][body]=The entire update sequence is wrapped in a \`BEGIN … ROLLBACK\` block, so nothing is ever committed. While this is fine as a smoke-test for the update script's SQL validity, it means the test never verifies that the extension actually reaches the expected version after the update.

Consider adding a version assertion before the \`ROLLBACK\`:
\`\`\`sql
SELECT is(
  (SELECT extversion FROM pg_extension WHERE extname = 'cat_tools'),
  :'EXPECTED_VERSION',
  'Extension updated to expected version'
);
\`\`\`
(Requires passing \`EXPECTED_VERSION\` into the test, e.g. via \`\pgxntool/psql.sql\` or a \`\set\` before \`\if\`.)" \
  --field "comments[][path]=HISTORY.asc" \
  --field "comments[][line]=17" \
  --field "comments[][side]=RIGHT" \
  --field "comments[][body]=\`// TODO\` placeholder — the 0.3.0 Changes section is empty. This is a release changelog; filling it in before merge will help users understand what changed." \
  --field "comments[][path]=README.asc" \
  --field "comments[][line]=23" \
  --field "comments[][side]=RIGHT" \
  --field "comments[][body]=This line still says **"Works on Postgres 9.3 and above"** but this PR drops PG10 and PG11 support and requires **PG12+**. Should be updated to reflect the new minimum version." \
  --field "comments[][path]=sql/cat_tools.sql.in" \
  --field "comments[][line]=1614" \
  --field "comments[][side]=RIGHT" \
  --field "comments[][body]=Trailing whitespace after \`pg_class\` on this line (and on line 1627 for \`relation__is_catalog\`). These are inside a dollar-quoted SQL string so it's not harmful, but it's inconsistent with the rest of the file style and will appear in the deployed function source." \
  --field "comments[][path]=sql/cat_tools--0.3.0.sql.in" \
  --field "comments[][line]=383" \
  --field "comments[][side]=RIGHT" \
  --field "comments[][body]=These inline comments — \`-- composite type (but mapped to materialized view)\` — are confusing because they describe the mapping as wrong while it is in fact intentional (preserved for backward compatibility from the original cat_tools relkind enum definition). The comments suggest a bug when the behavior is by design.

Consider replacing with an explanation that makes the intent clear, e.g.:
\`\`\`sql
WHEN 'c' THEN 'materialized view'  -- historical: cat_tools.relation_type predates 'composite type'; kept for API compat
WHEN 'f' THEN 'composite type'     -- historical: ditto for 'foreign table'
WHEN 'm' THEN 'foreign table'      -- historical: ditto for 'materialized view'
\`\`\`
Or, if this is actually an unintentional inversion that should be fixed, this is the right time to do so with a version bump." \
  2>&1

echo "Done"

#!/bin/bash
set -euo pipefail

REPO="Postgres-Extensions/cat_tools"
PR=16
SHA="434ef45ba69986a6cb4561302ea12a0067dca115"

tmpfile="$(mktemp)"
cat > "$tmpfile" <<'JSON'
{
  "event": "REQUEST_CHANGES",
  "commit_id": "434ef45ba69986a6cb4561302ea12a0067dca115",
  "body": "Review Summary\n\nThis is a substantial and well-structured release: the new enum taxonomy for pg_proc fields, the relation helper functions, the cross-version CI matrix, and the dynamic permissions test suite are all strong improvements.\n\nThere is one blocking issue (a committed test failure in test/expected/routine__.out) and several smaller bugs/doc gaps that should be addressed before merge.\n\nBlocking\n- test/expected/routine__.out contains a psql ERROR (see inline). Root cause: enum_mapping.sql calls :s.routine__type(:'sample_char') with an untyped literal; ambiguous between enum input and implicit 'char' cast. Fix by casting :'sample_char'::\"char\" and regenerate the expected file.\n\nBugs\n- Missing REVOKE EXECUTE FROM PUBLIC in the 0.2.2→0.3.0 upgrade script before the GRANTs on _cat_tools helpers.\n- test/build/upgrade.sql wraps the update in a transaction and rolls back; there is no post-update version assertion.\n\nDocumentation\n- HISTORY.asc 0.3.0 'Changes' section is still TODO.\n- README.asc still says 'Works on Postgres 9.3 and above' but minimum supported is now PG12+ per this PR.\n\nStyle/minor\n- Trailing whitespace in the SQL body strings for relation__is_temp and relation__is_catalog.\n- Relkind mapping comments are confusing; either clarify the historical compatibility or realign if unintended.\n",
  "comments": [
    {
      "path": "test/expected/routine__.out",
      "side": "RIGHT",
      "line": 4,
      "body": "Blocking: committed test failure. Lines 4–8 are a verbatim psql ERROR (42725: function cat_tools.routine__type(unknown) is not unique), not a TAP ok line. Root cause: enum_mapping.sql calls :s.routine__:kind(:'sample_char') with an untyped literal when kind=type and sample_char='f', which is ambiguous between the enum overload and the implicit ('char' → enum) cast. Fix by casting the variable in enum_mapping.sql: :s.routine__:kind(:'sample_char'::\"char\"). Then regenerate routine__.out."
    },
    {
      "path": "sql/cat_tools--0.2.2--0.3.0.sql.in",
      "side": "RIGHT",
      "line": 168,
      "body": "Upgrade script grants EXECUTE on _cat_tools.function__arg_to_regprocedure and _cat_tools.function__drop_temp but does not first REVOKE FROM PUBLIC. The install script adds the REVOKE for defense-in-depth. Please add:\\n\\nREVOKE EXECUTE ON FUNCTION _cat_tools.function__arg_to_regprocedure(text, text, text) FROM PUBLIC;\\nREVOKE EXECUTE ON FUNCTION _cat_tools.function__drop_temp(pg_catalog.regprocedure, text) FROM PUBLIC;\\n\\nthen keep the GRANT to cat_tools__usage."
    },
    {
      "path": "test/build/upgrade.sql",
      "side": "RIGHT",
      "line": 21,
      "body": "The update runs inside BEGIN … ROLLBACK, so nothing is committed; the test never asserts the resulting version. Consider asserting version before ROLLBACK, e.g.:\\n\\nSELECT is((SELECT extversion FROM pg_extension WHERE extname = 'cat_tools'), :'EXPECTED_VERSION', 'Extension updated to expected version');\\n\\n(You already set EXPECTED_VERSION in CI via cat_tools.control; piping it here would make this a stronger check.)"
    },
    {
      "path": "HISTORY.asc",
      "side": "RIGHT",
      "line": 17,
      "body": "TODO placeholder — please populate the 0.3.0 Changes section before merge so users have a proper changelog."
    },
    {
      "path": "README.asc",
      "side": "RIGHT",
      "line": 23,
      "body": "This still states 'Works on Postgres 9.3 and above' but this PR drops PG10 and PG11 (min is PG12+). Please update Supported Versions accordingly."
    },
    {
      "path": "sql/cat_tools.sql.in",
      "side": "RIGHT",
      "line": 1614,
      "body": "Trailing whitespace after 'FROM pg_catalog.pg_class' (and similarly at line 1627). It's inside a dollar-quoted string so harmless, but cleaning it avoids churn in diffed function definitions."
    },
    {
      "path": "sql/cat_tools--0.3.0.sql.in",
      "side": "RIGHT",
      "line": 383,
      "body": "These comments read like the mapping is wrong, but it looks intentional for backward compatibility with the historical cat_tools.relation_type values. Consider clarifying along the lines of: 'historical mapping preserved for API compatibility' for 'c'→'materialized view', 'f'→'composite type', 'm'→'foreign table'. If this inversion is unintentional, this would be the moment to correct with a versioned change."
    }
  ]
}
JSON

# Post the review
gh api "repos/${REPO}/pulls/${PR}/reviews" --method POST --input "$tmpfile"

echo "Review submitted."